ci(sdk): add proto drift detection and sync notifications - #3123

Why are the tasks are named following a different scheme ? I would propose to use also sdk:go:proto:drift for the naming of the golang drift task.

I've probably would use a more template like approach (i.e. a template with placeholder that are filled in with the data), then this very rudimentary and fragile list approach. That way it would be much nicer to see already how the report would look like eventually and is also easier to adapt. Not sure what Python offers here out of the pocket, but every language has this kind of templates (go as part of its std library). Your agent should be easily capable of converting this to a template based approach (templates typically also allow inside loops, too, e.g. for rendering the tables)

+1 on the template approach. For a CI utility script, a full template engine like Jinja2 would be overkill though. A good middle ground: define the template as a multiline string with .format() and pre-render the conditional/loop sections into simple strings before substituting.

ISSUE_TEMPLATE = """\
## Proto Drift Report

**Summary**: {summary}

{file_table}

{build_section}

## Fix Commands

```bash
mise run {proto_task}    # Regenerate bindings
mise run {build_task}    # Verify build
mise run {test_task}     # Run tests

Agent Instructions

{agent_section}
"""

def generate_issue_body(drift_report, build_report, sdk):
paths = SDK_CONFIGS[sdk]
drifted = [f for f in drift_report.get("files", []) if f.get("status") != "synced"]

file_table = _render_file_table(drifted) if drifted else ""
build_section = _render_build_section(build_report) if build_report else ""
agent_section = _render_agent_section(sdk, drifted, build_report)

return ISSUE_TEMPLATE.format(
    summary=drift_report.get("summary", "unknown"),
    file_table=file_table,
    build_section=build_section,
    proto_task=paths["proto_task"],
    build_task=paths["build_task"],
    test_task=paths["test_task"],
    agent_section=agent_section,
)

The template shape is immediately visible at the top of the file (you can see the final markdown structure at a glance), the conditional sections are isolated into small helper functions that each return a string, and there are zero new dependencies. The sub-sections (file table, agent prompt) are still built in Python, but they're small enough that it doesn't matter.

as said above, I would align the naming convention along side the typescript task (and add a sdk: prefix)

The tests only check for golang based drifts. Would it make sense to add some typescript-based tests ?

Important: The failure branch always reports "typecheck failed after proto regeneration" regardless of which step actually failed. The && short-circuits: if mise run sdk:ts:proto fails, mise run sdk:ts:typecheck never runs, but the summary blames typecheck.

Why this matters: Creates false-positive drift issues from transient CI failures (e.g., npm install timeout). The misleading summary sends investigators down the wrong path, wasting debugging time on proto drift that doesn't exist.

Suggested fix: Run the two steps separately to report which one actually failed:

if ! mise run sdk:ts:proto > "$LOG_FILE" 2>&1; then
  jq -n -c '{sdk:"typescript", synced:false, files:[], summary:"proto generation failed"}'
  exit 1
fi
if ! mise run sdk:ts:typecheck >> "$LOG_FILE" 2>&1; then
  jq -n -c '{sdk:"typescript", synced:false, files:[], summary:"typecheck failed after proto regeneration"}'
  exit 1
fi

Source: correctness agent

Important: No timeout-minutes set on any job. The sdk_sync_check job runs buf generate, full SDK builds, and test suites for both Go and TypeScript sequentially on linux-amd64-cpu8 runners. GitHub Actions defaults to 6 hours.

Why this matters: A hanging step (npm registry timeout, test deadlock, buf plugin stall) occupies an expensive 8-core runner for up to 6 hours. For the daily cron, this also blocks the entire issue_management pipeline since it depends on sdk_sync_check.

Suggested fix: Add timeout-minutes per job:

sdk_sync_check:
  timeout-minutes: 30  # builds + tests for two SDKs

# In sdk-proto-check.yml:
sdk_proto_drift:
  timeout-minutes: 15  # drift detection only

issue_management:
  timeout-minutes: 5   # just gh CLI calls

Source: production agent

Important: This new go:proto:drift task shares ~70% of its logic with the existing go:proto:check task (lines 138-178): tool availability checks, tmpdir creation with trap, sed-based buf.gen.yaml template substitution, buf generate invocation, and the two-pass file comparison loop. Only the output format (JSON vs human-readable text) differs.

Why this matters: If the buf generation approach or template substitution pattern changes, both tasks must be updated independently. A change to one without the other will cause silent divergence in drift detection results.

Suggested fix: Extract the shared logic (tool checks, tmpdir, buf generate, file diffing) into a shared script in tasks/scripts/ that accepts an --output-format flag (json or text). Both tasks delegate to it. Alternatively, refactor go:proto:check to output JSON and add a thin wrapper for human-readable formatting.

Source: architecture agent

Minor (defense-in-depth): The fixed heredoc delimiter REPORT_EOF for multiline GITHUB_OUTPUT values is a documented CI/CD antipattern. If $REPORT ever contained a line that is exactly REPORT_EOF, the heredoc would terminate early, allowing injection of arbitrary step outputs.

Why this matters: Current mitigations (compact JSON output, jq validation) make exploitation unlikely, but they protect against this by accident, not by design. If the output format changes (e.g., dropping -c, adding diagnostic lines), the protection disappears silently. This pattern appears 6 times across both workflow files.

Suggested fix: Use a randomized delimiter:

DELIM="REPORT_EOF_$(openssl rand -hex 8)"
{
  echo "report<<$DELIM"
  echo "$REPORT"
  echo "$DELIM"
} >> "$GITHUB_OUTPUT"

Source: security agent

Minor: NDJSON_FILE is named after "Newline-Delimited JSON" but stores tab-separated values (printf '%s\t%s\t%s\n'). The jq -R invocation later parses it as raw text split by tabs.

Why this matters: A maintainer adding a new field may assume each line is a JSON object and write jq filters that fail silently.

Suggested fix: Rename to DRIFT_TSV or RESULTS_FILE.

Source: architecture agent

Minor: After jq outputs the JSON report (which already contains "synced": true/false), the script re-reads $NDJSON_FILE with wc -l to determine the exit code. This is a redundant read of information already computed by jq.

Why this matters: A reader must trace two independent code paths (jq filter and wc) to verify they agree on drift status. If someone refactors the jq filter, the wc -l count could diverge.

Suggested fix: Capture jq output, extract synced, and use it for exit code:

REPORT=$(jq ... "$NDJSON_FILE")
echo "$REPORT"
SYNCED=$(echo "$REPORT" | jq -r '.synced')
[ "$SYNCED" = "true" ] && exit 0 || exit 1

Source: architecture agent

Minor: NDJSON_FILE=$(mktemp) creates a temp file, but the EXIT trap only cleans up WORK_DIR. If jq fails and set -e terminates the script, this temp file leaks.

Why this matters: Low impact (CI containers are ephemeral), but violates the cleanup pattern established by the existing trap.

Suggested fix: Update the trap after creating the file:

NDJSON_FILE=$(mktemp)
trap 'rm -rf "$WORK_DIR"; rm -f "$NDJSON_FILE"' EXIT

Source: correctness agent

Minor: _ensure_label calls _run_cmd(["gh", "label", "create", ...]) but discards the return value. If label creation fails (permissions, rate limit), manage_issue then tries gh issue create --label <non-existent-label>, which returns a 422. The error IS eventually propagated, but the root cause is obscured.

Why this matters: The operator sees "Failed to create issue" instead of "Failed to create label," making debugging harder on first run against a new repo.

Suggested fix: Check the return code and log:

result = _run_cmd(["gh", "label", "create", ...], capture=True)
if result.returncode != 0:
    print(f"Warning: failed to create label '{label}': {result.stderr}", file=sys.stderr)

Source: correctness agent

Minor: _run_cmd wraps subprocess.run without a timeout parameter. All gh CLI calls (label view/create, issue list/edit/create) make HTTP requests to the GitHub API.

Why this matters: During a GitHub API outage, any gh call could hang indefinitely. Combined with no job-level timeout in the workflow, this could tie up a runner for 6 hours.

Suggested fix: Add a timeout parameter with a reasonable default:

def _run_cmd(cmd, cwd=None, capture=False, stdin_data=None, timeout=60):
    return subprocess.run(cmd, cwd=cwd, capture_output=capture,
                          text=True, input=stdin_data, timeout=timeout)

Source: production agent

Minor: The sdk_sync_check job uses hardcoded per-SDK steps and outputs rather than a matrix strategy. Adding a third SDK requires duplicating step blocks (~25 lines of YAML), not just "a mise task + config entry" as the issue acceptance criteria state.

Why this matters: The PR-triggered workflow (sdk-proto-check.yml) delivers on extensibility via matrix, but the dashboard workflow does not. This is partly a GitHub Actions limitation (matrix outputs can't be dynamically named).

Suggested fix: Document the workflow modification steps needed to add a new SDK in a code comment, or restructure using artifact-based output passing.

Source: goal-alignment agent